feat(cve-scan): add reusable action to scan container images for cves - #213

Panel review: a nine-lane pass over the new cve-scan action. This is a non-blocking COMMENT review — nothing here gates the merge button.

Solid, well-documented action with a genuinely swappable adapter seam and a real bats suite. The findings below cluster around one theme: because a scanner error is designed never to fail the job, every prerequisite gap and internal error degrades into a permanently green, permanently non-scanning job. Several of these I reproduced by running the scripts rather than reading them; where that's the case the comment says so.

Blocking concerns

Highest-impact first. Each is a real defect, not a style preference.

1. src/process-findings.sh:99 — the expired-ignore ::warning:: never reaches the job log. Verified end to end: run the pair with an expired ignore entry and the job log is completely empty while high-count=1 shows the suppressed CVE has returned. run.sh captures the child's whole stdout as a key=value channel and re-emits only grepped keys. The one signal telling a human a CVE came back is destroyed. One-word fix (>&2).
2. src/process-findings.sh:121 — the jq partition's exit status is unchecked, so the gate silently no-ops. Verified: with a findings JSON the filter can't iterate, the script still exits 0, every count is written blank, scanner-error=false, and block-on-findings=true does not fire. A data error is indistinguishable from a clean scan.
3. action.yml:87-94 — the snyk CLI is never installed or version-pinned. yq gets a conditional install step; the scanner the action exists to run gets none, and isn't on ubuntu-latest. Exit 127 folds into the silent scanner-error path, so the action reports "scanner error" forever and never scans.
4. src/scanners/snyk.sh:48 — no image pull or registry auth. The design ticket for this work explicitly records that Snyk's own GHCR credential has broken repeatedly and that a CI scan should pull the image itself; the manual runbook does exactly that. As written, private-image scanning depends on the very credential this was meant to route around, and fails silently.
5. run.sh:69 — the enabled kill switch silently skips on any non-exact value. Verified: TRUE, True, yes, 1, and " true" all skip the scan; only lowercase true runs it. The examples wire this to a repo variable — a free-text web-UI field. Result is a green check and no annotation. (An empty value is safe — ${ENABLED:-true} re-defaults it.)
6. test/run.bats:57-67 — the mock only ever emits clean key=value lines, so nothing exercises the parent's handling of other child stdout. This is why #1 ships green.
7. test/process_findings.bats:149-165 — the severity-rank table is only ever exercised at high; ranking critical below low passes all 23 tests.
8. test/run.bats:141-153 — the gate matrix never varies block-on-findings where it matters; two mutations that break release gating pass the suite.

What was checked

• Correctness: boundary/empty inputs, ignore-file expiry and id/cve matching, the gate decision, temp-file and trap lifecycle. Expiry logic, zero-padding validation, and id/cve matching are correct; the unchecked jq exit above is the real defect.
• Security: injection, secrets in logs, authz. Clean on the important ones — untrusted scanner data reaches jq only via --arg/--argjson, never interpolated into program text, and the CLI invocation uses a proper argv array. Three defense-in-depth notes inline. Permission footprint is genuinely minimal and the SARIF-upload split is the right call.
• Test quality: exercised by mutation testing — production code was deliberately broken and the suite re-run to prove what it catches. Four gate-coverage gaps are blocking above.
• Operability: the disabled / scanner-error / config-error / completed taxonomy versus what an operator can actually observe. This is where the action is weakest; see blocking #1, #3, #4, #5 plus inline notes on step-summary and timeouts.
• Architecture: the adapter seam is real and the canonical shape is documented in one place. One leak remains (severity vocabulary) — inline.
• Reuse: compared against govulncheck and ~30 siblings. The inline output/table/truncation helpers are per-action convention here, not missed reuse; no shared helper exists to call. Two genuine items inline.
• Infra/CI: third-party actions correctly SHA-pinned; renovate annotations verified against the actual renovate.json regex; the notify step's compound if:/ternary verified correct across all reachable output states; empty webhook-url already handled safely by ci-test-notify.
• Typos/prose: doc-comment count mismatch and the set +e intent inline.
• PR metadata: below.
• Skipped: dead-code-guard (nothing deleted, moved, or renamed — 9 added files plus two append-only edits). The four e2e lanes and gap-analysis (no e2e*/ suite in this repo and no Go unit test, so neither signal is present).

Not re-reported: anything actionlint, zizmor, shellcheck, or check-docs already covers — all five checks are green. The ci-test-notify SHA pin is correct per this repo's convention and is deliberately not flagged.

What the change does well

The scanner seam is honest rather than nominal: the canonical findings shape is documented as an explicit adapter contract, and the Snyk-specific two-array/dedup trap is handled inside the adapter where it belongs instead of leaking into the gating logic.

PR-level notes

• nit — The body says "41 bats tests"; the PR now has 53 (23 + 15 + 15). 41 was the count at the first commit; the second added 12 more. The claim undersells the suite rather than overstating it. Everything else checks out: all five checks green, and References DEVOPS-1292 is the right form since this is Phase 2 and does not close the ticket.

Quality notes (non-blocking)

Low-severity test-assertion gaps, recorded for tracking rather than as inline comments:

• test/run.bats:167 — only critical-count is asserted from the real grep/cut output parsing; hardcoding high/medium/low-count to 0 still passes the suite.
• test/process_findings.bats:291-297 — the Slack summary test checks only truncation, never the embedded per-severity counts; swapping which count fills the critical= field still passes.

Panel review: second pass over cve-scan, at 0d0419b. Nine lanes ran. This is a non-blocking COMMENT review — nothing here gates the merge button.

First, the round-one response holds up. I checked all 24 resolved threads against the code rather than the replies, and every one is genuinely fixed; several are fixed better than what was suggested. The two places you pushed back — declining the mock-based test for the swallowed warning because the mock's stdout is swallowed either way, and deferring the severity enum until a second adapter lands — were right, and I'm not re-litigating them. The four things you found that the review missed are all real. None of the findings below is a re-raise.

What most of them have in common: the second commit moved a lot of code, and the new surface it created is where the gaps are. The gate bypass, the four unwritten config-error exits, the enabled gate in the manifest, and the vendor-named scanner-version default all arrived after round one.

Blocking concerns

1. run.sh:213 — the release gate can be made to pass on a real critical finding. The gate greps $GITHUB_OUTPUT back and takes tail -n1; process-findings.sh:207 writes summary= last, interpolating trigger-context unchecked. Reproduced end to end: with trigger-context=$'release\nhas-vulnerabilities=false' and one genuine critical finding, block-on-findings: true exits 0. Round one closed this class for image-ref; trigger-context reaches the same sink and is documented as a free-text label, which invites wiring it from event data.
2. src/process-findings.sh:207 — the sink side of the same defect. Unvalidated trigger-context in a single-line key=value append forges arbitrary step outputs. Worth closing at both ends: the read-back is fragile regardless of who can write to the file.
3. test/scanners_snyk.bats:173 — install_snyk()'s success path has no test. Every case either pre-seeds snyk on PATH or drives a failure branch, so the path every fresh runner takes — download, verify, chmod +x, execute — is unexercised, as is the no-published-checksum branch. This is the code that fetches and runs a binary over the network.
4. .github/workflows/test-cve-scan.yaml:53 — the smoke job short-circuits before it proves anything. enabled: false returns at run.sh's first branch, so 10 of 15 inputs get no manifest-wiring coverage. That's the one layer the bats suite structurally cannot see, and it's why this job was added in round one — so the fix is incomplete rather than wrong.

What was checked

• Correctness: boundary and empty inputs, the errexit behaviour of the read/here-doc/command-substitution construct, the trap/trap - pair, the ignore-file expiry and id/cve matching, adapter exit-code classification. The expiry logic, zero-padding validation, SARIF_URI fallback, ENABLED/BLOCK_ON_FINDINGS case loop, and install_snyk's dest-reuse and chmod ordering were each tested directly and are correct. Two real gaps inline (an unobserved jq exit, duplicate ignore ids collapsing).
• Security: injection, secrets in logs, authz, supply chain. Untrusted scanner data reaches jq only via --arg/--argjson/file, never interpolated into program text; the adapter resolution is enumerable; the permission footprint is minimal and leaving SARIF upload to the caller is the right call. Four findings inline, the trigger-context sink being the sharp one.
• Test quality: exercised by mutation, with bats 1.14.0 and a real mikefarah/yq installed. All six mutation-catch claims in the PR body hold — I broke each one and confirmed the suite fails. The two bare ! grep negations are not inert (both are the final statement in their body, so they're enforced), the loop-based tests bind correctly to their iteration, the two-array Snyk dedup trap is covered, and the curl-badsum stub's checksum format matches a live downloads.snyk.io fetch. Gaps are coverage, not rigor.
• Operability: all four outcomes traced to what a human actually sees. The four early config-error exits write neither a summary nor a Job Summary entry — reproduced, and it contradicts the README's "every outcome writes a Job Summary entry" — plus an unbounded docker pull whose job-timeout cancellation escapes the Slack if:, and a scheduled example that contradicts the README's own noise advice.
• Architecture: the adapter seam is genuinely real, not nominal. One vendor leak remains in the manifest (scanner-version) and the validation split now spans two files with two exit-code conventions. Inline.
• Reuse: compared against ~28 siblings. The per-action output/summary helpers, the docker/login-action SHA, the workflow shape, and the Makefile glob all match existing convention exactly — deliberately not flagged. normalize_flag/to_bool and the checksum-verified CLI install are genuinely first-of-their-kind here. One real item (test helpers duplicated three times under two names, where this repo already has a helpers.bash pattern).
• Infra/CI: every third-party pin, the intra-repo ci-test-notify SHA, and the renovate annotations verified against sibling files and the actual renovate.json — all correct. The intra-repo SHA pin is this repo's convention and is deliberately not flagged. One finding: the manifest's enabled != 'false' gate disagrees with run.sh's normalisation.
• Typos/prose: four doc-vs-code contradictions, including a documented input that doesn't exist and a README section describing an architecture this PR removed.
• PR metadata: below.
• Skipped: dead-code-guard (nothing deleted, moved or renamed — 10 added files plus two append-only edits). The four e2e lanes and gap analysis (no e2e*/ suite in this repo and no Go unit tests, so neither the e2e nor the unit-crossover signal is present).

Not re-reported: anything actionlint, zizmor, shellcheck, check-docs or validate-renovate already covers — all six checks are green.

What the change does well

The adapter seam earns its keep: the canonical findings shape is documented as an explicit contract, the Snyk-specific two-array/dedup trap is handled inside the adapter where it belongs, and the second commit correctly pushed CLI provisioning down there too rather than into the manifest — so adding a scanner really is close to a one-file change.

PR-level notes

• nit — The body says "111 bats tests"; the suite at this head has 99 (run.bats 36, process_findings.bats 36, scanners_snyk.bats 22, integration.bats 5), confirmed by grep -c '^@test' and bats --count. 111 was accurate at ebde485; the two later commits that cut the action down and dropped five duplicate tests brought it to 99. Stale rather than fabricated, and the claim understates the suite — but this is the second round where the count has drifted, so it may be worth just not putting a number in the body. Everything else checks out: the title and all 12 commit subjects follow the convention, References DEVOPS-1292 is the right form since this is Phase 2 and doesn't close the ticket, there's no PR template in this repo to satisfy, the set -euo pipefail claim is accurate for all three scripts, and the claim about govulncheck/run.sh's inverse errexit trap is correct.

Panel review: third pass over cve-scan, at a6e705f. Nine lanes ran. This is a non-blocking COMMENT review — nothing here gates the merge button.

First, the round-two response holds up. All 45 threads are genuinely resolved, and the four you pushed back on — the manifest-level scanner-version default, the arm64 and TIMEOUT_BIN branch tests, the read-back mechanism itself, and the enabled gate's residual case-sensitivity — were reasonable calls that I am not re-litigating. None of the findings below is a re-raise.

What most of them have in common: the commits that deleted the ignore-file parser and the hand-rolled SARIF generator are a real improvement, but they left three kinds of residue — a fix applied at one call site instead of in the shared helper it moved into, outputs published before the step that can still invalidate them, and a canonical contract whose consumers went away with the code that read it.

Blocking concerns

Highest-impact first. Each was reproduced by running the code, not by reading it.

1. run.sh:97 — the $GITHUB_OUTPUT forgery class is back, through three other inputs. Round two closed it for trigger-context by guarding at the image-ref validation site rather than in finish_with_no_result / finish_with_config_error. Those helpers still interpolate caller input into an unguarded append, and three paths reach them unvalidated: the enabled=false short-circuit runs before the image-ref check, and scanner and dockerfile-path are never checked at all. Reproduced: ENABLED=false with a newline in image-ref forges has-vulnerabilities=true; a newline in scanner forges sarif-path. Last-value-wins means the forged line is the step's real output.
2. run.sh:187 — sarif-path and scanner-error=false are published before process-findings.sh can fail, then the EXIT trap deletes the file they point at. trap - EXIT only runs after a successful child. Reproduced: $GITHUB_OUTPUT ends up with scanner-error=false and a sarif-path to a deleted file. This contradicts the documented "config errors write no result outputs at all" and breaks the README's own upload-sarif caller snippet.
3. test/integration.bats:75 — the seam this file exists for is not covered. Its header says it exists so the two scripts meet over the real $GITHUB_OUTPUT, but no test sends a finding through. Replacing the gate decision with a hardcoded false — disabling release gating completely — leaves all three tests green.
4. test/scanners_snyk.bats:373 — the adapter's severity mapping is asserted nowhere. Hardcoding severity: "high" for every finding leaves all 82 tests green. It is the only field the gate reads.
5. test/scanners_snyk.bats:410 — fixedIn is asserted nowhere either. Nulling it leaves all 82 tests green. This is the field DEVOPS-1292's second acceptance criterion depends on.

What was checked

• Correctness: the four outcomes against every exit path, set -euo pipefail behaviour across command substitution / || / if-tested contexts, the trap pair's coverage, adapter exit-code classification, and the three jq programs. The two-array combine and dedup, the counting pass, and the security-severity normalisation are each correct. The two real defects are blocking #1 and #2 above.
• Security: injection, secrets in logs, authz, supply chain. I re-verified all six round-two fixes empirically rather than trusting the threads — install_snyk now removes the partial download and exits 2 on both the mismatch and the unreachable-checksum paths, leaving no unverified binary to be reused by the cache check. Both ::add-mask:: sites run before anything can print. The SARIF jq is static, with no string-built program. The one finding is blocking #1.
• Test quality: exercised by mutation with bats 1.14.0 — production logic was broken one line at a time and the suite re-run. Three blocking gaps above, one consider, one quality note. The suite is strong on control flow and weak on translated values.
• Operability: all four outcomes traced to the three surfaces an operator has. A fifth state is unhandled — a job-timeout cancellation matches none of the Slack if: arms and runs none of the finish_with_* helpers, so the most likely unattended-cron failure pages nobody. Inline, with the unbounded docker pull that makes it reachable.
• Architecture: the adapter seam is real, and pushing CLI provisioning into it was right. Three leaks remain: the canonical contract now has one consumer for one of its six fields, suppression moved outside the seam into a vendor feature, and notify is the one switch with no normalisation.
• Reuse: compared against govulncheck and ~28 siblings. No production-logic duplication — there is no existing helper for checksum-verified binary download, boolean normalisation, or severity ranking anywhere in this repo. One real item: scanners_snyk.bats is the only suite that does not load the helpers.bash this PR just added.
• Infra/CI: clean, no findings. Every third-party pin matches its sibling usage byte-for-byte; the ci-test-notify intra-repo SHA pin is this repo's convention and is deliberately not flagged; the renovate annotation was checked by running this repo's actual renovate.json custom-manager regex against it rather than eyeballing the format; both smoke scenarios genuinely need no network or credential, so contents: read is sufficient.
• Typos/prose: one cross-reference mismatch. The README, the manifest descriptions and the script comments are otherwise consistent — notably no leftovers from the deleted ignore-file feature.
• PR metadata: below.
• Skipped: dead-code-guard (nothing deleted, moved or renamed relative to main — 11 added files plus two append-only edits). The four e2e lanes and gap analysis (no e2e*/ suite in this repo and no Go unit tests, so neither the e2e nor the unit-crossover signal is present).

Not re-reported: anything actionlint, zizmor, shellcheck, check-docs or validate-renovate already covers — all six checks are green.

What the change does well

Deleting the hand-rolled ignore parser and SARIF generator was the right call and took real discipline — roughly 110 lines, 26 tests and a yq dependency removed in favour of features the tool already has, on a branch that had already shipped them.

PR-level notes

• nit — The body says "81 bats tests"; grep -c '^@test' over test/*.bats returns 82 (run 36, process_findings 16, snyk 27, integration 3). The count has now drifted in all three rounds, always understating the suite — probably worth just not putting a number in the body.
• nit — The body describes the adapter as living at src/scanners/.sh; the <scanner> placeholder has been lost from the raw text. The README and action.yml both have it right.

Quality notes (non-blocking)

Recorded for tracking rather than as an inline comment:

• test/scanners_snyk.bats:211 — install_snyk's cache-reuse branch (the version-stamped early return) is unexercised; removing it entirely fails no test. Consequence is a ~180MB re-download per invocation rather than a wrong result, and it sits in the same category as the arm64 and TIMEOUT_BIN branches you already deferred.